fix: address all open CodeQL security alerts#480
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to remediate the repository’s open CodeQL security alerts by hardening response headers, constraining outbound GitHub API requests, and adding request rate limiting to the ReSpec size endpoints.
Changes:
- Add a sliding-window rate limiter utility and apply it to
/respec/sizeGET/PUT. - Constrain
routes/github/lib/utils/rest.tsto only fetchhttps://api.github.com/URLs (including pagination URLs). - Adjust security-related response headers (CSP
frame-ancestors,Content-Type: text/plainon certain error responses) and add tests/helpers to support the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/rate-limit.ts | Introduces sliding-window rate limiting middleware. |
| routes/respec/index.ts | Applies the new rate limiter to /respec/size routes. |
| routes/github/lib/utils/rest.ts | Adds allowlist validation for GitHub REST endpoints and pagination URLs. |
| routes/w3c/group.ts | Sets Content-Type: text/plain for invalid group-type 404 response. |
| routes/github/commits.ts | Sets Content-Type: text/plain for error responses to mitigate reflected XSS findings. |
| app.ts | Replaces frameguard: false with CSP frame-ancestors directive. |
| static/xref/filter/worker.js | Adds an inline comment on new Function() use (intended as suppression). |
| tests/utils/rate-limit.test.js | Adds unit tests for the new rate limiting middleware. |
| tests/routes/github/lib/utils/rest.test.js | Adds tests for the new GitHub API endpoint allowlist validation. |
| tests/jasmine.json | Registers a Jasmine helper to set env vars for tests. |
| tests/helpers/env.js | Sets env vars required by some modules at import/load time during tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/test.yml:24
- The workflow allows the TypeScript build step to fail (
continue-on-error: true), which means CI can go green even if new type errors are introduced (as long as tests still pass). Consider making the build step required (or at least adding a separate non-blocking step that uploads/logs thetscerrors) so type regressions don't get silently ignored.
continue-on-error: true
- run: pnpm test
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sidvishnoi
reviewed
Apr 15, 2026
Collaborator
Author
|
@copilot see @sidvishnoi's comments. Can you evaluate the suggestions from a security perspective and make the fixes? |
Contributor
Done in dcdfa50 and 65fec1e: tightened |
marcoscaceres
added a commit
that referenced
this pull request
Apr 15, 2026
* feat: target Node 24 LTS — bump .nvmrc, add engines field, CI on Node 24, drop node-fetch * refactor: replace node-fetch with native fetch, legacyDirname/Filename with import.meta, fix Express 5 param types * fix: apply XSS and SSRF security fixes from #480 * fix: source CI Node version from .nvmrc Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/5e94febe-bb8b-4d99-86e6-050ffa3f7cb0 Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com> * fix: address review feedback for node version source and commits route typing Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/dde0f0f2-efe1-43fc-b5df-d6bcdf6e4d53 * refactor: use typed Request<Params> in contributors and group routes * chore: apply reviewer suggestions for workflow actions and runtime checks Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/ccaf8248-161f-4e1c-be5b-c38f4c43bf24 * fix: add ImportMeta.main typing for CI TypeScript build Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/ddca0bfa-90d9-4424-a983-bdadec983e7d --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Collaborator
Author
|
@copilot rebase? |
Contributor
…e embedding alone
- Replace custom rate limiter with express-rate-limit (fixes memory exhaustion and IP spoofing bypass vectors) - Remove rate limit from PUT /respec/size (authenticated webhook route) - Tighten frame-ancestors from wildcard to 'self' + respec.org - Use URL origin comparison for SSRF check instead of prefix match - Add URL confusion test case for SSRF validation
- Rewrite assertGitHubAPIUrl to parse URL explicitly (no throw 0 trick) - Separate error messages for invalid URL vs wrong origin - Guard env helper with ??= to preserve developer's local config - Install express-rate-limit as direct dependency
- Use tryURL() helper in SSRF validation per Sid's suggestion, matching the codebase convention from utils/logging.ts. - Remove CSP frame-ancestors directive: the service intentionally allows embedding by any site for the ReSpec pill UI. - Update test to match unified error message.
Verify the express-rate-limit re-export works and returns middleware when called with valid options.
Tests call the middleware directly with mock req/res objects, verifying the site's rate-limiting behavior independent of which library implements it.
ucontent depends on the unmaintained html-minifier@4 which has a high-severity ReDoS vulnerability (dependabot alert #7). Override with the maintained fork html-minifier-terser@7 via pnpm overrides. API-compatible, no code changes needed.
Helmet's default CSP includes frame-ancestors: 'self' which blocks cross-origin iframe embedding. Delete the directive from defaults so the ReSpec pill UI can be embedded on any spec-hosting site.
Add protocol check to assertGitHubAPIUrl so blob: URLs (whose origin matches api.github.com) are rejected. Add positive test for valid URL, pagination URL validation test, and blob:/data: scheme rejection tests.
marcoscaceres
force-pushed
the
fix/codeql-security-alerts
branch
from
April 25, 2026 05:22
6471d07 to
4e12de1
Compare
- Extract tryURL to utils/misc.ts as shared utility (Sid: "can import existing tryURL") - Change env.js helper from ??= to ||= so empty-string env vars get defaults (Sid) - Fix error message wording: "endpoint origin must be" → "expected" (Sid: "origin includes protocol")
Node 22+ provides URL.parse() which returns null on invalid input instead of throwing — exactly what tryURL did. Remove the custom helper from both utils/misc.ts and utils/logging.ts.
Delete utils/rate-limit.ts (was just a re-export). Import express-rate-limit directly so CodeQL can trace the rate limiter.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes all 6 open CodeQL code-scanning alerts and resolves Dependabot alert #7 (html-minifier ReDoS).
routes/w3c/group.tsContent-Type: text/plainon 404 error responseroutes/github/commits.tsContent-Type: text/plainon 404 error responseroutes/github/lib/utils/rest.tstryURL()helper (not prefix match); same check on pagination URLsapp.tsframeguard: falseis intentional (service needs iframe embedding by any spec-hosting site for ReSpec pill UI)routes/respec/index.ts/respec/sizeviaexpress-rate-limit(10 req/min per IP)static/xref/filter/worker.jspnpm-lock.yamlhtml-minifierwithhtml-minifier-terser@7via pnpm overridesThe rate limiter applies only to the unauthenticated GET path. The PUT route is HMAC-authenticated (webhook) and does not need IP-based limiting.